fix(validator): isolate each NCCL benchmark run in its own namespace - #2443

ensureNamespace and waitForNamespaceGone can return a StructuredError
with a specific code (e.g. ErrCodeTimeout when a namespace is stuck
terminating). Wrapping their errors with a fresh ErrCodeInternal
flattened that code away instead of letting it propagate. Use
PropagateOrWrap so an already-coded error passes through unchanged,
falling back to ErrCodeInternal with the existing contextual message
only when it isn't one.

Signed-off-by: Mike Cook <micook@nvidia.com>

ensureNamespace treated any active, non-terminating namespace with the
derived per-run name as safe to adopt, and cleanupNCCLResources deleted
it unconditionally on exit. Since deriveRunID is deterministic for a
given AICR_RUN_ID, this let a same-run retry collide with (rather than
recover from) a still-live prior execution, and let cleanup delete a
namespace another run actively owns.

verifyNCCLNamespaceNotLive now fails closed with ErrCodeConflict when an
existing, non-terminating namespace still has a non-terminal pod in it,
before ensureNamespace is ever called. cleanupNCCLResources pins its
delete to the namespace UID captured right after creation/reclaim, so a
namespace deleted and recreated under the same name in between can't be
removed by the wrong run. createUnstructured now reclaims (Get + Update)
an AlreadyExists TrainingRuntime/TrainJob left by a crashed same-run
retry instead of failing, matching the pattern already used for
ComputeDomain and the RoCE ResourceClaimTemplate.

Signed-off-by: Mike Cook <micook@nvidia.com>

The namespace-cleanup defer was registered before ensureTrainerInstalled
so it also covers an install failure. But deleteTrainer's defer was
registered after it, so LIFO order ran deleteTrainer first, then the
namespace delete. On the self-install fallback (recipe does not declare
kubeflow-trainer), deleteTrainer tears down the Trainer controller and
its TrainJob/TrainingRuntime CRDs before the namespace's own
TrainJob/TrainingRuntime CRs are deleted. That can leave those CRs'
controller-serviced finalizers stuck forever and hang the namespace in
Terminating until cleanupNCCLResources's wait times out, turning a
green benchmark into a failure.

Replace the two separately-registered defers with a single
cleanupNCCLRun helper, registered once before installedResources is
even known (so an early ensureTrainerInstalled failure still cleans up
the namespace), that always deletes the namespace first and only then
tears down a self-installed Trainer.

Signed-off-by: Mike Cook <micook@nvidia.com>

cleanupNCCLResources's post-delete wait folded a waitForNamespaceGone
timeout into the check verdict via foldCleanupError, but the Delete
call above it already succeeded. The wait exists only for teardown
observability, since the apiserver's cascading GC completes it
server-side regardless of whether we wait here. NVLS runs create a
ComputeDomain plus an auto-generated ResourceClaimTemplate whose
DRA/IMEX finalizers can legitimately take longer than the 5-minute
wait bound to clear, so this could fail an otherwise-passing benchmark
on the high-value dual-fabric GB200 path. It also diverged from the
sibling cleanupInferenceWorkload, which is fire-and-forget on delete.

Log the timeout instead of returning it. A genuine Delete call failure
(the one part of cleanup this run's own fixed-name resources can
actually poison for the next run) is still returned unchanged.

Signed-off-by: Mike Cook <micook@nvidia.com>

deriveRunID is deterministic per AICR_RUN_ID, and the three catalog
checks that share one aicr validate invocation (nccl-all-reduce-bw,
-net, -nvls) all derive the same AICR_RUN_ID. Keying the namespace on
runID alone gave all three the identical name and identical
fixed-name resources inside it. That is harmless today only because
intra-phase entries run serially and cleanup fully drains a variant's
namespace before the next one starts, but pkg/validator has a
TODO(perf) to parallelize intra-phase entries, which would put two
variants in the same namespace concurrently and let the first to
finish delete the still-running sibling's namespace out from under it.

Add ncclRunNamespace(variant), used by both runNCCLTrainJob and the
regression tests, to fold the variant into the namespace name so each
check's isolation no longer depends on execution order.

Signed-off-by: Mike Cook <micook@nvidia.com>

A standalone aicr validate run (no AICR_RUN_ID) derives a random
namespace suffix. If the process is killed before its own deferred
cleanup runs, that aicr-nccl-perf-* namespace is orphaned: nothing
ever names it again, so nothing ever reclaims it. The Job path does
not have this problem since its run ID is deterministic and gets
reclaimed on retry.

Add a best-effort prune that runs before each NCCL benchmark starts:
list aicr-nccl-perf-* namespaces older than twice the TrainJob
timeout (well past any legitimately in-progress run) and delete them.
Deletion is fire-and-forget, so it adds no wall-clock cost to the run
in progress.

Signed-off-by: Mike Cook <micook@nvidia.com>

start was captured after the goroutine that fires the second delete
was already launched, so elapsed could in principle dip marginally
under holdFinalizer if the goroutine's 200ms sleep began before start
was assigned. Move the capture before the go statement.

Also note in a comment that deleteCalls reaching 2 alone doesn't prove
cleanup blocked. The goroutine fires that second delete
unconditionally regardless of what cleanupNCCLResources does. The
elapsed-time assertion right after it is the real guard.

Signed-off-by: Mike Cook <micook@nvidia.com>

waitForPodByLabelSelector returned whatever pod object arrived on the
next watch event, without checking the event type or the pod's phase.
A retry that recreates the workload under the same label selector
(e.g. the TrainJob admission retry) can fire a Deleted event for the
stale launcher before the replacement's Added event arrives, so the
caller could get handed a pod that no longer exists.

Skip Deleted events and pods that are already terminating or in a
terminal phase, matching the same filter newestRunnablePod already
uses on the watch-channel-closed recovery path, so the wait continues
until the actual replacement launcher shows up.

Signed-off-by: Mike Cook <micook@nvidia.com>

CodeRabbit review on PR NVIDIA#2443: createUnstructured reclaimed any
AlreadyExists fixed-name resource, including TrainJob, by updating it
in place. Kubeflow Trainer treats most of the TrainJob spec as
immutable once created and rejects an in-place update to those
fields, and even a permitted update would not make the controller
recreate the underlying JobSet and pods, which is exactly what a
same-run retry needs after a hard kill left a stale TrainJob behind.

Reclaim TrainJob by delete then recreate instead. TrainingRuntime
keeps the existing update-in-place reclaim, split into its own test
now that the two paths differ.

Signed-off-by: Mike Cook <micook@nvidia.com>

… pod

CodeRabbit review on PR NVIDIA#2443: pruneStaleNCCLNamespaces decided purely
on name prefix, DeletionTimestamp, and age, with no occupancy check.
A foreign run's namespace that ages past
defaults.NCCLStaleNamespacePruneAge while still running would be
deleted along with its live pods.

Reuse verifyNCCLNamespaceNotLive, the same occupancy predicate the
adoption gate already uses, and skip the delete when it reports a
live pod.

Signed-off-by: Mike Cook <micook@nvidia.com>

CodeRabbit review on PR NVIDIA#2443: cleanupNCCLResources passed uid
straight into the Delete call's UID precondition without checking it.
An empty UID is still an exact-match precondition against the real
namespace's UID, which is never empty, so it would only ever fail the
delete outright against a real apiserver. But the fake client used in
this package's tests ignores delete preconditions entirely and would
silently proceed, masking a caller bug that drops the UID.

Reject an empty UID explicitly instead of relying on that difference.
Give the namespace fixtures a real UID across the affected tests, and
add a UID-mismatch test using a reactor that emulates the real
apiserver's precondition check, since the fake ObjectTracker ignores
it.

Signed-off-by: Mike Cook <micook@nvidia.com>

CodeRabbit review on PR NVIDIA#2443: TestWaitForNamespaceGone_TimesOutWhenNeverDeleted's
doc comment described a caller behavior that no longer exists. It said
the wait boundedly fails the caller on timeout, but cleanupNCCLResources
now only logs that timeout and returns nil by design, so a slow-but-real
teardown does not fail an otherwise-passing benchmark.

Rewrite the comment to describe waitForNamespaceGone's own bounded-wait
contract directly instead of a caller behavior that was already changed.

Signed-off-by: Mike Cook <micook@nvidia.com>